-
Notifications
You must be signed in to change notification settings - Fork 952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make get_values
and alias of get
#1296
Conversation
update get docstring also update get_all_values docstring
to test that ValueRanges are proper maybe this should be tested for every input into `get` since it could be returned early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can make it simpler, see comment.
Changing the outcome of the test is good sing of a breaking change 😮 .
We should add a new warning about it.
I agree about the duplicate method. about the linting: I think we can solve it using the propose changes in the code. |
Yes, good that this is 6.0.0 ;). Do you think the breaking change is too big?
Not sure what you mean by "linting" |
assigning changed values back to response object
not too big, just may be it's too many. I don't know, I feel like this is a lot of changes.
I meant typing sorry 🙊 but you solved it by applying the proposed code change |
Yes, this is a big change. It needs some thinking about. The alternate options are:
|
For maximum compatibility, |
I propose the breaking change here that:
Then, the only difference between |
I agree with this change, but the default behavior must stay so:
this way we keep the same behavior as before but we allow user to padd at both levels. Then we'll be able to deprecate the old behavior in the next+2 major release if we want to
can we add some flags again to offer the choice of the returned value ? default: keep the old return type, flag is set: return ValueRange ? *I'm sorry I edited your comment instead of replying ! 🤦 * |
This is now ready to merge. Technically, this introduces no changes to the public API (👻wooo👻) I have removed all code from Lines 393 to 402 in 6627b53
All heavy lifting is now done in the You may notice that
They cannot be combined into one kwarg for two reasons:
|
This reverts commit da058b8.
and get_all_values
added #1303 into this PR... |
closes #1285, #1303
Having both of these method is ambiguous. I have made it so only
get
is the real method, andget_values
is its alias.Potential problems:
get_values
used to return typeList[List[str]]
.get
returns aValueRange
thus now
get_values
returns aValueRange
insteadValueRange
subclasseslist
, so it's okay?